Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Actionable Observability] rules page read permissions & snoozed status & no data screen #128108

Merged
merged 22 commits into from
Mar 24, 2022

Conversation

mgiota
Copy link
Contributor

@mgiota mgiota commented Mar 18, 2022

Fixes #123585 -> must have for 8.2
Fixes #127434 -> must have for 8.2 (this depends on #123585, so it had to be pushed in this open PR)
Fixes #127583 -> nice to have (this depends on #123585, so it had to be pushed in this open PR)

Acceptance criteria

Before
Screenshot 2022-03-22 at 10 17 26

After
Authorized user
Screenshot 2022-03-22 at 10 13 54

Unauthorized user
Screenshot 2022-03-22 at 13 58 27

cc @katrin-freihofner

@mgiota mgiota self-assigned this Mar 21, 2022
@mgiota mgiota marked this pull request as ready for review March 21, 2022 07:40
@mgiota mgiota added v8.2.0 Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" labels Mar 21, 2022
@mgiota
Copy link
Contributor Author

mgiota commented Mar 21, 2022

@elasticmachine merge upstream

@mgiota mgiota changed the title 123585 rules page read permissions [Actionable Observability] rules page read permissions Mar 21, 2022
@mgiota mgiota added the release_note:skip Skip the PR/issue when compiling release notes label Mar 21, 2022
@mgiota mgiota requested a review from fkanout March 21, 2022 08:02
@mgiota mgiota requested a review from a team as a code owner March 21, 2022 11:17
@mgiota mgiota changed the title [Actionable Observability] rules page read permissions [Actionable Observability] rules page read permissions & snoozed status Mar 21, 2022
@mgiota mgiota requested a review from XavierM March 22, 2022 07:36
@mgiota
Copy link
Contributor Author

mgiota commented Mar 22, 2022

@XavierM @katrin-freihofner I updated Snoozed label to Snoozed permanently as per our discussion
Screenshot 2022-03-22 at 08 35 27

@XavierM Can you send me the ticket of your team with the new snooze api?

cc @vinaychandrasekhar

@mgiota mgiota changed the title [Actionable Observability] rules page read permissions & snoozed status [Actionable Observability] rules page read permissions & snoozed status & no data screen Mar 22, 2022
@mgiota
Copy link
Contributor Author

mgiota commented Mar 22, 2022

@elasticmachine merge upstream

@XavierM
Copy link
Contributor

XavierM commented Mar 22, 2022

@XavierM @katrin-freihofner I updated Snoozed label to Snoozed permanently as per our discussion Screenshot 2022-03-22 at 08 35 27

@XavierM Can you send me the ticket of your team with the new snooze api?

cc @vinaychandrasekhar

That's our PR #128214 and please do not hate me but it looks like that we call it snooze indefinitely

Screen Shot 2022-03-22 at 5 24 00 PM

@mgiota
Copy link
Contributor Author

mgiota commented Mar 22, 2022

it looks like that we call it snooze indefinitely

@XavierM I renamed to indefinitely. Thanks for the link to your PR. I will keep an eye on it. If it is merged in time, I might be able to use the new snooze api.

@mgiota
Copy link
Contributor Author

mgiota commented Mar 22, 2022

I just had a look at your PR. If you expose the x-pack/plugins/triggers_actions_ui/public/application/sections/rules_list/components/rule_status_dropdown.tsx component in the start method https://github.com/elastic/kibana/blob/main/x-pack/plugins/triggers_actions_ui/public/plugin.ts#L190, I could easily reuse it.

I can expose your component, once your PR is merged. I don't want to push for it, since we didn't plan to deliver the new snooze api in the o11y rules page anyway. But if you make it in time, I can squeeze this in o11y as well for 8.2.

cc @katrin-freihofner @vinaychandrasekhar

@mgiota mgiota force-pushed the 123585_rules_page_read_permissions branch from de225e2 to b98f947 Compare March 23, 2022 09:54
@fkanout
Copy link
Contributor

fkanout commented Mar 23, 2022

@mgiota, I don't know exactly the expected behavior, so I will share this with you
I created a user with the next role privileges (APM all and read for the others):
Screenshot 2022-03-23 at 13 20 30

However, when I went to the rules pages trying to create an APM Error Count rule, I'm getting the next error
Screenshot 2022-03-23 at 13 27 27

And I can click save, but I will have the error rule status.
Is that normal? do I need more privileges to be able to create an APM rule?

@XavierM
Copy link
Contributor

XavierM commented Mar 23, 2022

I just had a look at your PR. If you expose the x-pack/plugins/triggers_actions_ui/public/application/sections/rules_list/components/rule_status_dropdown.tsx component in the start method https://github.com/elastic/kibana/blob/main/x-pack/plugins/triggers_actions_ui/public/plugin.ts#L190, I could easily reuse it.

I can expose your component, once your PR is merged. I don't want to push for it, since we didn't plan to deliver the new snooze api in the o11y rules page anyway. But if you make it in time, I can squeeze this in o11y as well for 8.2.

cc @katrin-freihofner @vinaychandrasekhar

That's the plan for 8.3, for 8.2 we were just making it internal. I do think that we should also focus our time to get the rules table re-usable between plugins. Therefore a lot of feature will come available to you for free. It is more work at the beginning but we will be more aligned in the long run.

@XavierM
Copy link
Contributor

XavierM commented Mar 23, 2022

@fkanout can you try to replace this
image
by Actions and Connectors All All?
I think you need to be able to have access to Actions and Connectors in the same space than your APM privileges

@fkanout
Copy link
Contributor

fkanout commented Mar 23, 2022

@XavierM, I updated the role privileges as suggested, the issue is still there.

But I realized something interesting, and I'm not sure if it's related to this PR. It seems there are ONLY 2 rule types from APM throwing this error: Failed transaction rate threshold , and Latency threshold.

cc @mgiota

@fkanout
Copy link
Contributor

fkanout commented Mar 23, 2022

@XavierM, I updated the role privileges as suggested, the issue is still there.

But I realized something interesting, and I'm not sure if it's related to this PR. There are ONLY 2 rule types from APM >throwing this error: Failed transaction rate threshold , and Latency threshold.

cc @mgiota

FYI, I tried the same thing with the same user (apm-all), but on Kibana main branch, the issue is there too (if it's an issue and not missing privileges). 🤔

const currentStatus = item.enabled ? RuleStatus.enabled : RuleStatus.disabled;
let currentStatus: RuleStatus;
if (item.enabled) {
currentStatus = item.muteAll ? RuleStatus.snoozed : RuleStatus.enabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have a rule enabled and muted at the same time?

I assume snoozed == muteAll, right? The API that provides the item is using muteAll

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fkanout Yep we are using muteAll for the snoozed (indefinitely) state. And yes a rule can be enabled and muted at the same time. I had similar questions here #123580 (comment) and we ended up with @katrin-freihofner on following:

  • If it is only enabled the Enabled state is shown in the UI
  • If it is snoozed, then the Snoozed indefinitely state is shown in the UI, and yep it is enabled as well.
  • To unmute/unsnooze user needs to select enabled (behind the scenes it enables and unmutes the rule)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgiota, excellent explanation, thanks!
snoozed + enabled will keep the rule but it will not generate alerts?

@mgiota
Copy link
Contributor Author

mgiota commented Mar 23, 2022

@XavierM, I updated the role privileges as suggested, the issue is still there.
But I realized something interesting, and I'm not sure if it's related to this PR. There are ONLY 2 rule types from APM >throwing this error: Failed transaction rate threshold , and Latency threshold.
cc @mgiota

FYI, I tried the same thing with the same user (apm-all), but on Kibana main branch, the issue is there too (if it's an issue and not missing privileges). 🤔

@fkanout For apm it looks like you need to have these predefined roles { "names": ["apm-*", "logs-apm*", "metrics-apm*", "traces-apm*"], "privileges": ["read", "view_index_metadata"] } as described here #90806. I will try it now as well

@mgiota
Copy link
Contributor Author

mgiota commented Mar 23, 2022

@fkanout I can confirm that everything works fine if you define following according to the predefined roles mentioned here #90806
Screenshot 2022-03-23 at 15 37 57

Without about mentioned predefined roles I get the error you mentioned both in the Stack Management rules page and the o11y rules page. So it is no problem with the code, but as you correctly guessed missing privileges and indices in the custom role configuration.
Screenshot 2022-03-23 at 15 34 58

@mgiota
Copy link
Contributor Author

mgiota commented Mar 23, 2022

I do think that we should also focus our time to get the rules table re-usable between plugins. Therefore a lot of feature will come available to you for free. It is more work at the beginning but we will be more aligned in the long run.

@XavierM I totally agree. The two hooks I already created (use_fetch_rules and use_load_rule_types) are moving towards this direction.

I already have a plan in my mind. After feature freeze and once we prioritize with the team, I can start working towards this direction.

@mgiota mgiota requested a review from fkanout March 23, 2022 14:51
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved changes in triggersActionUi plugin

Copy link
Contributor

@fkanout fkanout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just need to green the CI.

@mgiota
Copy link
Contributor Author

mgiota commented Mar 24, 2022

@elasticmachine merge upstream

@mgiota mgiota enabled auto-merge (squash) March 24, 2022 09:16
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 380 383 +3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
triggersActionsUi 273 277 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 415.9KB 419.4KB +3.5KB
triggersActionsUi 663.2KB 663.2KB +7.0B
total +3.5KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observability 90.2KB 90.2KB +52.0B
triggersActionsUi 96.6KB 96.7KB +177.0B
total +229.0B
Unknown metric groups

API count

id before after diff
triggersActionsUi 285 289 +4

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @mgiota

@mgiota mgiota merged commit 55d7476 into elastic:main Mar 24, 2022
@mgiota mgiota added the backport:skip This commit does not require backporting label Mar 28, 2022
@mgiota mgiota deleted the 123585_rules_page_read_permissions branch May 10, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.2.0
Projects
None yet
5 participants